Skip to content

Conversation

@mvorisek
Copy link
Contributor

@mvorisek mvorisek commented Nov 10, 2025

part of #19802

assert #20375 and #20437 using CI

The workflow YML file is generated to make adding/maintaining it easier.

@mvorisek mvorisek force-pushed the verify_bundled_boost_context branch from 5a94e72 to 623876d Compare November 12, 2025 18:00
@mvorisek mvorisek marked this pull request as ready for review November 12, 2025 18:01
@mvorisek mvorisek force-pushed the verify_bundled_boost_context branch 2 times, most recently from 39a8c6f to 0a2d4f7 Compare November 13, 2025 06:29
@kocsismate
Copy link
Member

I love the concept, thanks! Can you show an example failure?

The workflow YML file is generated to make adding/maintaining it easier.

I find it the opposite way: in its current form, I find it very difficult to understand now.

@mvorisek
Copy link
Contributor Author

mvorisek commented Nov 13, 2025

I love the concept, thanks! Can you show an example failure?

Thank you ❤

Using mvorisek@738c667 commit a failure is simulated - see CI results in https://github.com/mvorisek/php-src/actions/runs/19343856007/job/55338921670.

The workflow YML file is generated to make adding/maintaining it easier.

I find it the opposite way: in its current form, I find it very difficult to understand now.

Why, how can it be done better?

Maintaining the yml file by hand would not be practical as names/paths are copied in several places.

Adding a new bundle is easy - add 1 line to https://github.com/php/php-src/blob/94cdb07789/.github/scripts/download-bundled/make-workflow-file.php#L8-L12.

The script also generates partly the download scripts with canonical temporary directory names.

In case you would do any manual change to the yml by mistake, the CI at https://github.com/php/php-src/blob/94cdb07789/.github/actions/verify-generated-files/action.yml#L16 will warn you about.

Comment on lines +4 to +5
# use the repo root directory as "--git-dir"
cd "$(dirname "$0")/../.."
Copy link
Contributor Author

@mvorisek mvorisek Nov 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI only: This is important as the original cd "$(dirname "$0")/../../$1" did not detected any changes in case of the checked directory was containing .git directory.

Example: git clone https://github.com/derickr/timelib.git ext/date/lib

@mvorisek mvorisek requested a review from iluuu1994 November 14, 2025 23:27
@mvorisek
Copy link
Contributor Author

@TimWolla @iluuu1994 can this PR be merged?

@mvorisek mvorisek force-pushed the verify_bundled_boost_context branch from b92b445 to 95e4fac Compare November 23, 2025 11:42
@mvorisek mvorisek force-pushed the verify_bundled_boost_context branch from 95e4fac to e065fc1 Compare November 23, 2025 11:45
@mvorisek
Copy link
Contributor Author

Can this PR be merged please? The MacOS job failure is unrelated. I would like to continue another PR for more sources.

@iluuu1994
Copy link
Member

As mentioned, 2/3 of this PR is code that isn't needed. I don't think this should be merged in this state. There are other mechanisms to avoid duplicate code.

@mvorisek
Copy link
Contributor Author

mvorisek commented Nov 23, 2025

Thank you for your message.

I however do not think you can manage
a) the paths when the workflow should run on push
b) the paths for dorny changes detector
c) the unified scripts header

using anything than templating.

The only thing that can be deduplicated using "other mechanisms to avoid duplicate code" is the download/verify step.

Given all a), b), c) (or at least some of them) has no other solution, if we want to avoid mistakes and actual code duplication, the script is needed.

If you know how to avoid it without having to specify the paths/names more than once or twice only, please tell me.

Also please see #20487 and try adding a new definition/line into https://github.com/php/php-src/blob/94cdb07789/.github/scripts/download-bundled/make-workflow-file.php#L8-L12. You will see very quickly the power and benefits the templating script have.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants